Add Rust-to-Dart logging bridge#3079
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a logging bridge that routes Rust log records to the Dart logging package, including a new enable_frb_logging! macro and a Dart-side FrbDartLogging utility. Feedback focuses on several critical areas: the generated Dart initialization code needs to await asynchronous calls to prevent type errors, and the Rust logger initialization must handle Flutter Hot Restarts gracefully to avoid panics. Additionally, improvements were suggested for adding error handling to the Dart stream subscription and adopting more idiomatic log level mappings between the two languages.
| log::set_boxed_logger(Box::new(FrbDartLogger { sink })) | ||
| .map(|()| log::set_max_level(max_level)) | ||
| .expect("FRB logging should only be initialized once"); |
There was a problem hiding this comment.
Using .expect() on log::set_boxed_logger will cause the application to crash if frb_init_logger is called more than once. This is a critical issue for Flutter's Hot Restart, where the Dart state is reset but the Rust process (and its global logger) persists. Subsequent calls to init will trigger this panic.
Consider handling the error gracefully. Ideally, the logger should be initialized only once (e.g., using std::sync::Once), and the StreamSink should be stored in a static RwLock so it can be updated on each call to frb_init_logger to support re-connection after a hot restart.
There was a problem hiding this comment.
I did this on purpose: My thinking is that nobody would call frb_init_logger a second time intentionally.
Thus, if done accidentally, it should crash rather than silently swallow log messages.
If there is a reason for a second intentional init, the suggested changes sounds fine.
There was a problem hiding this comment.
@patmuk a bit worried about dart hot restart, thus maybe I will add a more general handling to it
|
docs LGTM |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3079 +/- ##
===========================================
+ Coverage 33.47% 98.24% +64.76%
===========================================
Files 51 473 +422
Lines 1198 19546 +18348
===========================================
+ Hits 401 19202 +18801
+ Misses 797 344 -453 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@fzyzcjy that worked great :) I left some comments. Tell me if I can help further! |
|
TODO: add tests |
Precommit AutofixCommit: CI produced no diff. The branch is already clean after precommit autofix. |
2 participants
continue from @patmuk #2766